Fixed assignment to catch block parameter #15384
Conversation
Signed-off-by: Dmitry Kryukov <dk2k@ya.ru>
dk2k
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15384 +/- ##
============================================
+ Coverage 71.93% 71.99% +0.06%
- Complexity 63223 63285 +62
============================================
Files 5214 5214
Lines 295938 295939 +1
Branches 42754 42754
============================================
+ Hits 212876 213074 +198
+ Misses 65601 65376 -225
- Partials 17461 17489 +28 ☔ View full report in Codecov by Sentry. |
|
Could really use a test on this path. |
|
This PR is stalled because it has been open for 30 days with no activity. |
Signed-off-by: Dmitry Kryukov <dk2k@ya.ru> (cherry picked from commit ebcf5e3) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit ebcf5e3) Signed-off-by: Dmitry Kryukov <dk2k@ya.ru> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Dmitry Kryukov <dk2k@ya.ru>
| : new OpenSearchException(e.getCause()); | ||
| Exception exception = e; | ||
| if (exception instanceof ExecutionException) { | ||
| exception = (exception.getCause() == null || exception.getCause() instanceof Exception) |
There was a problem hiding this comment.
@dbwiddis Does this logic make sense? If exception.getCause() is null, then we assign null to exception, which is going to NPE when we do throw exception; three lines later. Am I reading this right?
There was a problem hiding this comment.
@andrross I didn't fully study the logic, as it already existed.
The only change here was really making a copy of the e argument passed to the catch block and renaming all the other e to exception. It actually would have made a smaller diff had it been catch (Exception ex) followed by Exception e = ex and then no other changes.
That said, I see the former logic does include casting null as an Exception and then rethrowing it later. But all that said, will the cause of an ExecutionException ever be null in this situation? I don't think so.
There was a problem hiding this comment.
Agree this change is good by itself.
The existing logic seems to be quite wonky, but I think you're right that the contract of ExecutionException is that the cause will never be null.
There was a problem hiding this comment.
the contract of ExecutionException is that the cause will never be null.
I think you can explicitly create one with the no-param constructor. But I can't find any instances of that in the OpenSearch codebase, and most other handling of this instanceof don't even bother to check for null. So we could probably remove the null check. It's an NPE in either case, just a different spot in the code.
Signed-off-by: Dmitry Kryukov <dk2k@ya.ru>
Fixed assignment to catch block parameter